Skip to content

fix(extension): keep user-opened tabs free & fix agent-tab leak on session stop (issue #57) - #66

Open
iAstro wants to merge 3 commits into
Tencent:mainfrom
iAstro:fix/issue-57-user-tab-freedom
Open

fix(extension): keep user-opened tabs free & fix agent-tab leak on session stop (issue #57)#66
iAstro wants to merge 3 commits into
Tencent:mainfrom
iAstro:fix/issue-57-user-tab-freedom

Conversation

@iAstro

@iAstro iAstro commented Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

Fixes issue #57 — in the Agent-controlled browser window, tabs the user opens
themselves should stay fully under the user's control (not taken over by
BrowserSkill), and session stop should never leak an agent-created tab.

Three concrete fixes:

  1. Agent-tab leak on session_stop — tool.session_stop decided whether to
    release (keep) the window by checking "are there any tabs left?". A tab that
    failed to close was still present and got mistaken for a user tab → the
    window was released and the agent tab leaked. Now the release decision
    excludes any tab still tracked in agentCreatedTabs, so a failed-to-close
    agent tab forces the window to close instead.
  2. User-opened tabs are no longer taken over — added userTabs tracking plus
    a pendingAgentTabCount counter consumed by chrome.tabs.onCreated to tell
    apart "agent-created via tool.tab_create" from "user-created via Chrome UI".
    User tabs get a hidden overlay and never show the control mask.
  3. No control-mask flash on user tabs — added overlayStateForTab /
    pushOverlayStateForTab, which decide per-tab state before sending it, so a
    user tab receives hidden on its very first ping.

Post-review fixes (thanks @BB-fat)

  • Blocker 1: AgentWindowApi.ensureActiveTab now resolves with the tab id
    (Promise<number>) instead of Promise<void>, so SessionManager can record
    homeTabId and tsc typechecks.
  • Blocker 2: wired the real tabManagement.tabs / tabsQuery into
    tool.session_stop (dispatcher + disconnect cleanup). They were never
    injected, leaving the cleanup and window-release path dead outside tests.
    Added a dispatcher-level integration test that fails without the wiring.
  • Blocker 3: rebased onto the latest main (f06a514).
  • Nit 1: handleTabCreate releases the pending agent-tab slot when
    chrome.tabs.create fails (an abort is excluded — the tab did open), so the
    counter no longer leaks into the next user-opened tab.
  • Nit 2: dropped the unreachable windowInitializing branch; the home tab is
    matched by homeTabId, reliable regardless of onCreated event ordering.
  • Nit 3: refreshed the overlay-bridge docstring for the push-style
    overlay.ready.

Test plan

  • manager.test.ts: classifyNewTab returns agent for the home tab (matched
    by homeTabId) and for a pending tool.tab_create, user when nothing is
    pending; multiple pending agent tabs map to multiple onCreated events; the
    pending slot is released when tab creation fails.
  • session.test.ts: regression — an agent tab that fails to close forces the
    window to close (not release), so it cannot leak.
  • dispatcher.test.ts: integration — a surviving user tab releases the window
    instead of closing it (fails without the dispatcher wiring).
  • tabs.test.ts: regression — after a failed tab_create, a user-opened tab is
    still classified user (not agent).
  • All 811 extension tests, pnpm lint (179 checks) and ext:build pass locally.
  • Manual: in an Agent window, open a tab via Cmd+T and navigate → the tab stays
    free to operate (no control mask, no flash). On session_stop, agent tabs
    close and the window is released only when genuine user tabs remain.

@BB-fat

BB-fat commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Thanks @iAstro — the direction is right and the release-decision fix for #57 is clean, but there are 3 blockers before this can merge:

🔴 1. TypeScript doesn't compile: manager.ts does const homeTabId = await this.agentWindow.ensureActiveTab(...) and stores it as number | null, but AgentWindowApi.ensureActiveTab still returns Promise<void> (agent-window.ts:18 unchanged) — TS2322, tsc typecheck fails. Fix: change the interface to return Promise<number> and update chromeAgentWindowApi accordingly.

🔴 2. Core functionality is dead code in production: the session_stop rework depends on deps.tabManagement?.tabs and deps.tabsQuery, but dispatcher.ts:270-273 still calls handleSessionStop(this.sessions, params, { cdp: this.cdp }) — the new deps are never injected, so the release path never runs in production (behavior identical to main; the tests only pass because they inject fakes). Fix: wire the real Chrome APIs in the dispatcher, and add coverage at that integration level.

🔴 3. Conflicts with main: #52 changed manager.start(sessionId, size) / agentWindow.create(url, size); this PR is based on the old signatures. Needs a rebase.

🟡 Also worth fixing: pendingAgentTabCount leaks when chrome.tabs.create fails (misclassifies the next user-opened tab as agent — a variant of the very bug this PR fixes); the "initializing" branch in classifyNewTab is unreachable in production; the overlay.ready protocol change to push-style leaves the bridge docstring stale.

Once 1-3 are fixed this is valuable — happy to re-review! 方向没问题，修掉这三个卡点（尤其编译错误和 dispatcher 接线）后欢迎继续。

@iAstro
iAstro force-pushed the fix/issue-57-user-tab-freedom branch from f2a9b0a to f3529d4 Compare September 1, 2026 03:13
@Ljy-0827 @iAstro
fix(extension): keep user-opened tabs free & fix agent-tab leak on se…
a4fed13 
…ssion stop (issue Tencent#57)

- session_stop: exclude tabs still tracked in agentCreatedTabs from the
  release decision so a tab that failed to close is no longer mistaken for
  a user tab (which previously kept the window open and leaked the tab).
- Distinguish user-created tabs from agent-created tabs in the Agent Window
  via a pending-tab counter consumed by chrome.tabs.onCreated, so tabs the
  user opens themselves (new-tab button / Cmd+T / open in new tab) are never
  taken over by the BrowserSkill control mask.
- background overlay: decide per-tab state BEFORE showing controls
  (overlayStateForTab + pushOverlayStateForTab) so a user tab receives hidden
  on its first overlay.ready ping instead of flashing control then hiding.
- AgentWindowApi.ensureActiveTab now resolves with the activated/created tab
  id instead of void, so SessionManager can record homeTabId. This fixes the
  tsc failure flagged in review (the interface previously returned
  Promise<void> while callers treated the result as number | null).
- Wire the production tabManagement.tabs / tabsQuery deps into
  tool.session_stop, both in the dispatcher and in the disconnect cleanup.
  They were never injected, which left the agent-tab cleanup and the
  window-release path as dead code outside tests (behaviour identical to
  main). Added a dispatcher-level integration test that fails without the
  wiring: a surviving user tab must release the window, not close it.
@iAstro
iAstro force-pushed the fix/issue-57-user-tab-freedom branch from f3529d4 to a4fed13 Compare September 1, 2026 03:26
@iAstro
fix(extension): release the pending agent-tab slot when tab creation …
91621b2 
…fails

tool.tab_create increments pendingAgentTabCount *before* chrome.tabs.create
so the onCreated listener can tell an agent tab from a user-opened tab. When
the create failed, no onCreated event arrived and the counter leaked, so the
next tab the user opened was misclassified as agent-created — the exact
confusion the counter exists to prevent, a variant of issue Tencent#57.

handleTabCreate now releases the slot on failure via
SessionManager.releaseAgentTabPending. An abort (cancelled) is excluded: the
tab did open (and was cleaned up), so its onCreated still consumes the slot.

Added a regression test asserting that after a failed tab_create, a
user-opened tab is still classified as 'user' (not 'agent').
@iAstro
fix(extension): classify the home tab by id, not a racy boot flag (is…
a77353b 
…sue Tencent#57)

SessionManager.classifyNewTab's `initializing` branch relied on a
windowInitializing flag that start() cleared *before* the home tab's
chrome.tabs.onCreated event was dispatched (the event is asynchronous), so
the branch never ran in production and the home tab's classification was
subject to a timing race. Match the home tab by its tab id instead — the id
is already returned by ensureActiveTab and is reliable regardless of event
ordering.

Also refresh the overlay-bridge docstring: overlay.ready now makes the
background *push* the per-tab overlay state (and the Agent Window also
pushes on tab create / control-mode change), rather than a request/reply.
@iAstro

iAstro commented Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Thanks @iAstro — the direction is right and the release-decision fix for #57 is clean, but there are 3 blockers before this can merge:

🔴 1. TypeScript doesn't compile: manager.ts does const homeTabId = await this.agentWindow.ensureActiveTab(...) and stores it as number | null, but AgentWindowApi.ensureActiveTab still returns Promise<void> (agent-window.ts:18 unchanged) — TS2322, tsc typecheck fails. Fix: change the interface to return Promise<number> and update chromeAgentWindowApi accordingly.

🔴 2. Core functionality is dead code in production: the session_stop rework depends on deps.tabManagement?.tabs and deps.tabsQuery, but dispatcher.ts:270-273 still calls handleSessionStop(this.sessions, params, { cdp: this.cdp }) — the new deps are never injected, so the release path never runs in production (behavior identical to main; the tests only pass because they inject fakes). Fix: wire the real Chrome APIs in the dispatcher, and add coverage at that integration level.

🔴 3. Conflicts with main: #52 changed manager.start(sessionId, size) / agentWindow.create(url, size); this PR is based on the old signatures. Needs a rebase.

🟡 Also worth fixing: pendingAgentTabCount leaks when chrome.tabs.create fails (misclassifies the next user-opened tab as agent — a variant of the very bug this PR fixes); the "initializing" branch in classifyNewTab is unreachable in production; the overlay.ready protocol change to push-style leaves the bridge docstring stale.

Once 1-3 are fixed this is valuable — happy to re-review! 方向没问题，修掉这三个卡点（尤其编译错误和 dispatcher 接线）后欢迎继续。

Addressed everything from the review:

Blocker 1: ensureActiveTab now resolves with the tab id; homeTabId typechecks.
Blocker 2: wired tabManagement.tabs / tabsQuery into tool.session_stop (dispatcher + disconnect cleanup). Added a dispatcher-level test that fails without the wiring — a surviving user tab now releases the window instead of closing it.
Blocker 3: rebased onto the latest main (f06a514).
Nit 1: handleTabCreate releases the pending slot on create failure (no more counter leak misclassifying the next user tab).
Nit 2: dropped the unreachable windowInitializing branch; the home tab is matched by homeTabId (reliable regardless of event ordering).
Nit 3: refreshed the overlay-bridge docstring for the push-style overlay.ready. All 811 extension tests, lint, and build pass locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iAstro @BB-fat @Ljy-0827